Fixing AES encryptor cache - fix by argmarco#91
Conversation
sofia-tekdatum
requested review from
argmarco-tkd,
avalerio-tkd and
cristekdatum
|
Thanks for opening a pull request! If this is not a minor PR. Could you open an issue for this pull request on GitHub? https://github.com/apache/arrow/issues/new/choose Opening GitHub issues ahead of time contributes to the Openness of the Apache Arrow project. Then could you also rename the pull request title in the following format? or See also: |
avalerio-tkd
left a comment
avalerio-tkd
left a comment
There was a problem hiding this comment.
LGTM, just a few questions to clarify.
| uint64_t AesEncryptorFactory::MakeCacheKey( | ||
| ParquetCipher::type alg_id, int32_t key_len, bool metadata) { | ||
| uint64_t key = 0; | ||
| key |= static_cast<uint64_t>(static_cast<uint32_t>(alg_id)) << 32; |
There was a problem hiding this comment.
I absolutely love this!
Let's just add a comment for each bit encoded field.
| ParquetCipher::type alg_id, int32_t key_size) { | ||
| auto key_len = static_cast<int32_t>(key_size); | ||
| int index = MapKeyLenToEncryptorArrayIndex(key_len); | ||
| uint64_t cache_key = MakeCacheKey(alg_id, key_len, /*metadata=*/true); |
There was a problem hiding this comment.
Why is the last param commented out? /*metadata=*/true
There was a problem hiding this comment.
It's not, it's just the comment as to what the parameter is.
There was a problem hiding this comment.
Sorry, yes. Misread it.
|
|
||
| if (meta_encryptor_cache_[index] == nullptr) { | ||
| meta_encryptor_cache_[index] = AesEncryptor::Make(alg_id, key_len, true); | ||
| if (encryptor_cache_.find(cache_key) == encryptor_cache_.end()) { |
There was a problem hiding this comment.
this is essentially the same as before but in find/end format, right? Or is there more to it?
Let's add a comment either way.
There was a problem hiding this comment.
it's the same. Added comment.
| ParquetCipher::type alg_id, int32_t key_size) { | ||
| auto key_len = static_cast<int32_t>(key_size); | ||
| int index = MapKeyLenToEncryptorArrayIndex(key_len); | ||
| uint64_t cache_key = MakeCacheKey(alg_id, key_len, /*metadata=*/false); |
There was a problem hiding this comment.
Ah! We need a comment here since it's the critical part that changed.
There was a problem hiding this comment.
Added. Also check the comment in the .h file
2 participants
Adding fix by argmarco to the AES encryptor cache.
Small modification from his original fix -- including the write length in the key and modifying how the write length was sent caused all tests to fail.
Tests currently passing (all encryption-related tests).